Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make sure executeBatch returns error response for rows that would not get into database #503

Merged
merged 1 commit into from
Feb 7, 2016

Conversation

vlsi
Copy link
Member

@vlsi vlsi commented Feb 2, 2016

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502

@codecov-io
Copy link

Current coverage is 56.88%

Merging #503 into master will increase coverage by +0.14% as of d6e3b17

@@            master    #503   diff @@
======================================
  Files          143     143       
  Stmts        15060   15091    +31
  Branches      2950    2958     +8
  Methods          0       0       
======================================
+ Hit           8546    8585    +39
+ Partial       1172    1169     -3
+ Missed        5342    5337     -5

Review entire Coverage Diff as of d6e3b17

Powered by Codecov. Updated on successful CI builds.

@vlsi
Copy link
Member Author

vlsi commented Feb 3, 2016

@davecramer , @kjurka , @sehrope any volunteers to review the patch?

I think the patch is ready.

I've corrected behavior to silently accept SELECT statements in the batch set. I have no idea why it was rejected before. Is there a good reason to reject SELECTs in batch executions?

@reschke
Copy link

reschke commented Feb 3, 2016

Yes, this seems to work for me. In my test, it now returns -3 for all batch operations, and Oak (after removing the Postgres-specific workaround) copes with it.

Note that the RDBDocumentStoreJDBCTest in oak trunk (that I mentioned previously) will have to be modified to accept the Postgres behavior (failing all parts of the batch). See https://issues.apache.org/jira/browse/OAK-3977.

} catch (SQLException e) {
fail("Should throw a BatchUpdateException instead of " + "a generic SQLException: " + e);
updateCounts = stmt.executeBatch();
fail("0/0 should throw BatchUpdateException");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is division by zero

@vlsi
Copy link
Member Author

vlsi commented Feb 3, 2016

I'll refactor largeBatchUpdateFailureTransacted to explore more cases.

Something like that

  enum AutoCommit {
    YES, NO;
  }

  enum GeneratedKey {
    YES, NO;
  }

  enum FailMode {
    NO_FAIL, FAIL_VIA_SELECT, FAIL_VIA_DUP_KEY
  }

  enum FailPosition {
    FIRST_ROW, SECOND_ROW, LAST_ROW, ALMOST_LAST_ROW
  }

if (latestGeneratedRows != null) {
// We have DML. Decrease resultIndex that was just increased in handleResultRows
resultIndex--;
// If seen exception, no need to collect generated keys
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment is better worded as If exception thrown...

…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502, closes #503
@vlsi vlsi merged commit d6e3b17 into master Feb 7, 2016
@vlsi vlsi deleted the batch_with_errors branch February 7, 2016 12:10
@jkprasad
Copy link

This fix seems to break the following

I have a few insert statements batched for improving performance. With update count returning for whatever succeeded I had a chance to ignore the failed ones and try again the succeeded ones. Now there is no clue which one in the batch actually caused BatchUpdateException as it returns -3 for all inserts

Should executeBatch consider the failed ones as the changes anyway get rolled back if I roll back the transaction leaving no scope of partial commit?

@vlsi
Copy link
Member Author

vlsi commented Aug 29, 2017

Now there is no clue which one in the batch actually caused BatchUpdateException as it returns -3 for all inserts

@jkprasad , to my best understanding, JDBC specification provides no way to identify the row that caused the failure.
Please follow the discussion here: #670 (comment)

The suggested behavior is to retry rows with -3 status with reduced batch size. Can you use that approach? Any problems with that?

@jkprasad
Copy link

@vlsi Million thanks for the quick reply.
Good to know that it was thought about. I can try with reduced batch. By the way it's -3 for all statements, I didn't see any other value when an insert failed in a batch

@vlsi
Copy link
Member Author

vlsi commented Aug 29, 2017

By the way it's -3 for all statements, I didn't see any other value when an insert failed in a batch

That depends on the autocommit true/false setting (hint: with autocommit=true is safe to have certain rows committed, so it does occasionally commit). With autocommit=false, pgjdbc has to mark all rows as rolled back since no row will be able to hit the DB due to PostgreSQL's behavior "transaction has error status, you must rollback".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BatchUpdateException: incorrect update counts
5 participants